Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard Type Hint Imports Used Only Inside Annotations #12488

Closed

Conversation

notatallshaw
Copy link
Member

@notatallshaw notatallshaw commented Jan 25, 2024

I'm submitting this change because I recently implemented it in my company's codebase and found it beneficial. I understand though if Pip maintainers choose not to adopt it.

This PR utilizes ruff to introduce from __future__ import annotations as a mandatory import. This allows all flake8-type-checking rules to be enabled, via ruff, to relocate imports used solely as annotations into a type-checking import guard block.

Motivation:

  • Type hints are not meant for runtime use.
  • Type hinting in Pip can be challenging, passing type checking and having valid annotations at runtime from Python 3.7 to 3.12.
  • Reduces the likelihood of circular imports.
  • Provides minor performance improvements.

The commits are as follows:

  1. Adding a ruff rule in pyproject.toml to require from __future__ import annotations
  2. Applying the ruff rule to the Pip codebase using ruff check . --fix
  3. Adding flake8-type-checking ruff rules to pyproject.toml
  4. Applying "TCH001: Move application imports into a type-checking block" to the Pip codebase using ruff check . --select TCH001 --fix --unsafe-fixes
  5. Applying "TCH002: Move third-party imports into a type-checking block" to the Pip codebase using ruff check . --select TCH002 --fix --unsafe-fixes
  6. Applying "TCH003: Move standard library imports into a type-checking block" to the Pip codebase using ruff check . --select TCH003 --fix --unsafe-fixes

No manual touch-ups were applied in these commits. However, I've observed that some "else" blocks in "if TYPE_CHECKING" can be removed after the above commits.

Finally, note that from __future__ import annotations can be removed when Pip only needs to support Python versions with PEP 649, which would not be before October 2029.

@notatallshaw notatallshaw changed the title Guarding Type Hinting Imports Used Only Inside Annotations Guard Type Hint Imports Used Only Inside Annotations Jan 25, 2024
@pfmoore
Copy link
Member

pfmoore commented Jan 25, 2024

I'm -0 on this. Given that from __future__ import annotations was rejected in favour of PEP 649, I'm not particularly keen on using it as a temporary workaround, particularly when we do actually have type annotations in place that work.

Without some specific benefit that we can't get any other way (and I consider "not annotating a problem function" to be a valid "other way") I don't think we should do this. But honestly, I find it difficult to care about type annotations, so I'm not going any further than -0.

@notatallshaw
Copy link
Member Author

notatallshaw commented Jan 25, 2024

I'm -0 on this. Given that from __future__ import annotations was rejected in favour of PEP 649, I'm not particularly keen on using it as a temporary workaround, particularly when we do actually have type annotations in place that work.

During type checking phase there is no difference between from __future__ import annotations and PEP 649.

During runtime, for most usecases, there is a bigger difference between PEP 649 and not having from __future__ import annotations on non-PEP 649 versions of Python. As PEP 649 has no runtime effect unless you're inspecting the annotations, which also from __future__ import annotations has no runtime effect unless you're inspecting (though how you inspect is perhaps different).

Therefore I've moved most by codebase to include from __future__ import annotations as I beleive it will be easier supporting multiple versions of Python that have PEP 649 and non-PEP 649 behavior.

Without some specific benefit that we can't get any other way (and I consider "not annotating a problem function" to be a valid "other way") I don't think we should do this.

I don't 100% follow this, but Pip already has to do a lot of if TYPE_CHECKING: for the reasons I already outlined. In fact in PRs I've recently submitted (both accepted and pending review) I had make choices on what to do about type hints that were non-obvious.

With these rules most choices would have become obvious, if it's an annotation only then put it in a type checking block that's import guarded. I then don't have to worry about if the type was introducted in Python 3.8 or if I should create some fake runtime object (which exists in places in the Pip codebase right now).

I find it difficult to care about type annotations, so I'm not going any further than -0.

This is not a hill I'm going to die on, I just found it useful in my own codebases. I've outlined the exact steps I did to create this PR, it can easily be revisited if my predictions of supporting Python versions across PEP 649 and non-PEP 649 come true, by myself or someone else.

@notatallshaw
Copy link
Member Author

notatallshaw commented Jan 25, 2024

One additional benefit, which this commit highlited: 39a910a

Is it prevents importing objects for runtime uses from a module that was only importing that object for type hint annotations, which seems like a strong code smell to me.

@notatallshaw
Copy link
Member Author

Closing this PR due to expressed non-intesterst from Pip maintainers, and as it affects so many files it will inevetibly code rot and it should not be manually fixed but rather the automated steps I described should be reapplied on clean codebase: #12488 (comment)

As a quick summary if anyone wants to revist this again, the benefits are:

  • Standardized and enforced use of if TYPE_CHECKING:
  • Simpler workflow for devs dealing with annotations that may not be present on all versions of Python or risk introducing circular imports
  • Greater compatability between pre PEP 649 and post PEP 649 Python
  • Minor runtime performance benefits

Further cleans ups could also be acheived:

  • Drop "fake" runtime annotations currently in a few places in the Pip codebase

@notatallshaw notatallshaw deleted the guard-all-type-hinting-imports branch January 29, 2024 23:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants